Skip to content

[GEN][ZH] Change reference counter implementation for StateMachine to avoid mismatch with Retail#1078

Merged
xezon merged 6 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-statemachine-retail-mismatch
Jun 17, 2025
Merged

[GEN][ZH] Change reference counter implementation for StateMachine to avoid mismatch with Retail#1078
xezon merged 6 commits into
TheSuperHackers:mainfrom
xezon:xezon/fix-statemachine-retail-mismatch

Conversation

@xezon

@xezon xezon commented Jun 17, 2025

Copy link
Copy Markdown

This changes the reference counter implementation for StateMachine to avoid mismatch with Retail.

The previous reference counter changes the struct layout of StateMachine, which then changed the generated assembler code of many functions that interact with StateMachine.

TODO

  • Test and confirm that this fixes the observed mismatch
  • Replicate in Generals

@xezon xezon added this to the Stability fixes milestone Jun 17, 2025
@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Fix Is fixing something, but is not user facing Stability Concerns stability of the runtime labels Jun 17, 2025
Comment thread GeneralsMD/Code/GameEngine/Include/Common/StateMachine.h
@Caball009

Copy link
Copy Markdown

I just checked again with this branch and the change works as intended: replay 366648 no longer mismatches and produces the same score overview as retail.

@xezon

xezon commented Jun 17, 2025

Copy link
Copy Markdown
Author

Replicated in Generals

@Caball009

Copy link
Copy Markdown

Is the crash that 890 fixed originally still fixed, could you verify that?

@xezon

xezon commented Jun 17, 2025

Copy link
Copy Markdown
Author

Is the crash that 890 fixed originally still fixed, could you verify that?

Tested. Is fixed.

WWASSERT(NumRefs != IntegerType(0));
if (--NumRefs == IntegerType(0))
{
deleteFunction(const_cast<ObjectType*>(objectToDelete));

@Caball009 Caball009 Jun 17, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of having a const function and mutable member variable? I see the consistency with RefCountClass, but I don't see why it's needed there either.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required so that const objects can still do reference counting. Otherwise we cannot pass around const objects, or destroy them.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense if those const objects use a mutable RefCountValue member variable?

@xezon xezon Jun 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const_cast needs to be somewhere. I think it is better if it is buried here in this class and not in all code that interacts with this class.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the const_cast need to be anywhere if the RefCountValue is mutable?

struct RefCountValue
{
	int NumRefs = 0;

	void Release_Ref()
	{
		--NumRefs;
	}
};

struct StateMachine
{
	void Release_Ref() const
	{
		m_refCount.Release_Ref();
	}

	mutable RefCountValue m_refCount;
};

int main()
{
	const StateMachine machine;
	machine.Release_Ref();
}

Is this not a representative minimal example?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, const cast happens when passing this pointer to delete function.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Maybe you can add a comment for RefCountValue::Release_Ref that explains the use of const member function and const_cast on the object that'll be deleted.

@xezon xezon Jun 17, 2025

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Added a bunch of comments in EA/Westwood comment style.

@xezon xezon merged commit 2b74bda into TheSuperHackers:main Jun 17, 2025
18 checks passed
@xezon xezon deleted the xezon/fix-statemachine-retail-mismatch branch June 17, 2025 22:07
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Feb 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Fix Is fixing something, but is not user facing Gen Relates to Generals Major Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple replays that mismatch with main branch

2 participants